Fix TLS certificate validation bypass in non-cURL HTTP path (CWE-295)#276
Open
mynetx wants to merge 1 commit into
Open
Fix TLS certificate validation bypass in non-cURL HTTP path (CWE-295)#276mynetx wants to merge 1 commit into
mynetx wants to merge 1 commit into
Conversation
Two separate bugs in the non-cURL (file_get_contents/stream context) request path, only reachable when the curl extension is unavailable or setUseCurl(false) is called explicitly: 1. _getNoCurlInitialization() nested the SSL options under $httpOptions['ssl'], which array_merge_recursive() then placed at $options['http']['ssl']. PHP's https:// stream wrapper only reads SSL context options from the top-level 'ssl' key, so the intended cafile/verify_depth/peer_name settings were silently ignored. 2. _fetchRemoteFile() explicitly set a top-level 'ssl' => ['verify_peer' => false] for its media-download request, unconditionally disabling certificate validation. Confirmed exploitable with a local TLS server presenting a self-signed certificate for the correct hostname. Fix: - Keep the SSL options as a sibling top-level context key instead of nesting them under 'http', and assign them directly rather than through the recursive merge (a future caller passing its own 'ssl' key could otherwise turn a boolean into an array on collision). - Remove the explicit verify_peer => false override in _fetchRemoteFile(), so it inherits the secure defaults. - Pass the request hostname into _getNoCurlInitialization() from _fetchRemoteFile() (via parse_url), matching the other two call sites. Without it, peer_name became an explicit empty string instead of being unset, which broke certificate validation for every HTTPS download, not just malicious ones. Caught while verifying the first version of this fix against a real HTTPS URL. - Set verify_peer_name explicitly, matching PHP's own 5.6+ default. - Bump the composer.json PHP floor from 5.5.0 to 5.6.0, since peer_name/automatic hostname verification require it (5.5 only had the deprecated CN_match option). The README already documents PHP 7.1+ as the real requirement. Verification: the project's PHPUnit suite doesn't run under PHPUnit 13 (legacy test-class naming isn't discovered), so this was verified with a standalone script using reflection to call both methods against a local openssl s_server: - self-signed certificate, hostname-matching but untrusted: rejected by both _getNoCurlInitialization() and _fetchRemoteFile() - real HTTPS request (https://www.google.com/robots.txt): succeeds through both, confirming no regression on legitimate requests Fixes jublo#275
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #275
Two bugs in the non-cURL (
file_get_contents/stream context) request pathOnly reachable when the curl extension is unavailable or
setUseCurl(false)is called explicitly._getNoCurlInitialization()nested the SSL options under$httpOptions['ssl'], whicharray_merge_recursive()then placed at$options['http']['ssl']. PHP'shttps://stream wrapper only reads SSL context options from the top-levelsslkey, so the intendedcafile/verify_depth/peer_namesettings were silently ignored._fetchRemoteFile()explicitly set a top-level'ssl' => ['verify_peer' => false]for its media-download request, unconditionally disabling certificate validation. Confirmed exploitable with a local TLS server presenting a self-signed certificate for the correct hostname.Fix
http, and assign them directly rather than through the recursive merge (a future caller passing its ownsslkey could otherwise turn a boolean into an array on collision).verify_peer => falseoverride in_fetchRemoteFile(), so it inherits the secure defaults._getNoCurlInitialization()from_fetchRemoteFile()(viaparse_url), matching the other two call sites. Without it,peer_namebecame an explicit empty string instead of being unset, which broke certificate validation for every HTTPS download, not just malicious ones. Caught while verifying the first version of this fix against a real HTTPS URL.verify_peer_nameexplicitly, matching PHP's own 5.6+ default.composer.jsonPHP floor from 5.5.0 to 5.6.0, sincepeer_name/automatic hostname verification require it (5.5 only had the deprecatedCN_matchoption). The README already documents PHP 7.1+ as the real requirement, so this doesn't change what's actually supported in practice.Verification
The project's PHPUnit suite doesn't run under PHPUnit 13 (legacy test-class naming isn't discovered by the modern loader), so this was verified with a standalone script using reflection to call both affected methods against a local
openssl s_server:_getNoCurlInitialization()and_fetchRemoteFile()https://www.google.com/robots.txt): succeeds through both, confirming no regression on legitimate requests🤖 Generated with Claude Code